feat: infer country from member location (CM-1355) - #4434
Conversation
Signed-off-by: Yeganathan S <[email protected]>
66b8d20 to
5928c63
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The attribute helper signatures break data-sink type checking, and dependency loading and unrelated lockfile churn need correction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds country inference from member location across ingestion, enrichment, and backend update paths.
Changes:
- Adds country parsing, aliases, ambiguity filtering, and attribute helpers.
- Applies inferred countries across member create/update flows.
- Adds
i18n-iso-countries.
File summaries
| File | Description |
|---|---|
services/libs/common/src/member.ts |
Adds attribute value helpers. |
services/libs/common/src/index.ts |
Exports country utilities. |
services/libs/common/src/country.ts |
Implements country inference. |
services/libs/common/src/constants/location.ts |
Defines location mappings and exclusions. |
services/libs/common/src/constants/index.ts |
Exports location constants. |
services/libs/common/package.json |
Adds country dependency. |
services/apps/members_enrichment_worker/src/activities/enrichment.ts |
Infers country during enrichment updates. |
services/apps/data_sink_worker/src/service/member.service.ts |
Infers country during ingestion. |
backend/src/services/memberService.ts |
Adds inference to legacy update paths. |
backend/src/services/member/memberAttributesService.ts |
Adds inference to attribute updates. |
pnpm-lock.yaml |
Resolves the dependency and incidental lockfile changes. |
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
services/libs/common/src/member.ts:90
- For the same reason,
attributes.countryisunknownin both new data-sink paths and is not assignable to this parameter, causing type checking to fail. Narrow anunknownvalue inside the helper instead.
export function hasAttributeValue(attribute: Record<string, any> | null | undefined): boolean {
return Object.values(attribute || {}).some((v) => typeof v === 'string' && v.trim().length > 0)
- Files reviewed: 10/11 changed files
- Comments generated: 3
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The English locale is not registered, so the new country lookups currently return no inferred country.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (4)
services/libs/common/src/member.ts:69
- This new shared helper exposes
anyeven though every value is explicitly narrowed before use. Useunknownso callers cannot consume unchecked values through this API, as required by the repository's type-safety guidance.
attribute: Record<string, any> | null | undefined,
services/libs/common/src/member.ts:89
- This second shared helper also introduces
anyunnecessarily;Object.valuesplus the existingtypeofguard works withunknownand preserves type safety.
export function hasAttributeValue(attribute: Record<string, any> | null | undefined): boolean {
pnpm-lock.yaml:10957
- These AWS peer-resolution rewrites (along with the changed
globdeprecation metadata) are unrelated to addingi18n-iso-countriesand can change dependency resolution outside this feature's scope. Regenerate with the repository's expected pnpm version or revert the unrelated lockfile hunks while retaining only the new dependency entries.
services/libs/common/src/country.ts:1 i18n-iso-countriesdoes not load name data automatically. NoregisterLocalecall exists in the repository, so bothgetAlpha2Code(..., 'en')andgetName(..., 'en')return no result and this inference path never resolves a country. Import the English locale and register it once in this module before using the lookup APIs.
import countries from 'i18n-iso-countries'
- Files reviewed: 10/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Signed-off-by: Yeganathan S <[email protected]>
There was a problem hiding this comment.
🟡 Not ready to approve
Inferred countries can be marked as manual, and the configured priorities do not enforce the documented system precedence.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (2)
backend/src/services/memberService.ts:831
- This inferred value is added before
MemberRepository.update, but the UI member update calls this method withmanualChange: true. The repository treats every changed attribute default as manual (memberRepository.ts:794-845), so this system-derived country is persisted asattributes.countryinmanuallyChangedFieldsand later automated updates cannot replace it. Keep generated country data out of manual-change detection.
data.attributes.country = {
...data.attributes.country,
system: country,
default: data.attributes.country?.default ?? country,
}
services/apps/members_enrichment_worker/src/activities/enrichment.ts:369
- The claimed
system-over-enrichmentprecedence is not configured:getPlatformPriorityArrayreturns settings unchanged, while the default priority list inbackend/src/database/models/settings.ts:34-46omitssystem. Once a later enrichment country is merged, default recomputation selectsenrichmentand overwrites this inferred default. Addsystemahead ofenrichmentfor both default and existing persisted settings, or explicitly encode the precedence.
attributes.country = {
...attributes.country,
system: country,
}
- Files reviewed: 10/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Signed-off-by: Yeganathan S <[email protected]>
There was a problem hiding this comment.
🟡 Not ready to approve
Inferred countries can become stale and may be incorrectly marked as manually changed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (3)
services/libs/common/src/member.ts:96
- Counting
systemanddefaulthere makes an inferred country permanent. After the location changes, every new guard sees the old inferredcountry.system/defaultas an existing value and never recomputes it, leaving the member with a stale country. Distinguish authoritative external/custom sources from the inferred system value so location updates can refresh the inference.
return Object.values(attribute as Record<string, unknown>).some(
(v) => typeof v === 'string' && v.trim().length > 0,
)
backend/src/services/memberService.ts:830
- When this inference runs during the UI member update (
manualChange: true),MemberRepository.updatesees the newly addedattributes.countryand records it inmanuallyChangedFields. The value was generated by the system, not the user, and that flag causes later non-manual updates to preserve it as if it were user-owned. Exclude the inferred field from manual-change tracking or perform inference after the manual fields are determined.
data.attributes.country = {
...data.attributes.country,
system: country,
default: data.attributes.country?.default ?? country,
services/libs/common/src/member.ts:76
- On merged update paths this
defaultis stale until the subsequentsetAttributesDefaultValuescall. For example, changing a source location from Germany to France leavesdefault: "Germany"during inference, so the code writes Germany as the country and then refreshes the location default to France. Resolve the priority-selected location before inferring (and then populate the country default) rather than preferring the pre-merge default here.
if (typeof record.default === 'string' && record.default.trim()) {
return record.default
- Files reviewed: 10/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Summary
Infers a member country from free-text location when no country source is already set, and writes it to
attributes.country.systemon create/update paths.Changes
getCountry(location)in@crowd/common(i18n-iso-countries+ curated junk/ambiguous/US-state/alias maps); abstains when unsuregetAttributeValue/hasAttributeValuehelpers for reading multi-source attributessetAttributesDefaultValueson data-sink create/update, enrichment attribute updates, backend UI attributes PATCH, and legacy member upsert/updatedefaultwhen inferring (no priorities pass on that path)